Feat: allow passing cronjob specific settings #12
Conversation
| source "https://rubygems.org" | ||
|
|
||
| gem "kubeclient", "4.0.0" | ||
| gem "kubeclient", "4.12.0" |
There was a problem hiding this comment.
Bumped to latest due to vulnerability note:
https://github.com/ManageIQ/kubeclient?tab=readme-ov-file#vulnerability-in--v492
There was a problem hiding this comment.
Looking at this PR, it seems like there are a ton of aesthetic/rubocop-y type changes that are insubstantial as far as the primary aim of the PR goes.
PRs are much easier to review and discuss if you separate aesthetic changes into a different PR that has no functional changes. That way contributors can discuss aesthetics/style without holding up valuable functional changes, which I think this PR probably has.
Also, I'd love to see new test cases that prove the new functionality that was added works as intended
|
|
||
| gemspec | ||
|
|
||
| group :development, :test do |
There was a problem hiding this comment.
Is there some reason you moved these out of the gemspec? I seem to recall that keeping dev dependencies in the gemspec is more standard / better practice
| spec.homepage = "https://github.com/keylimetoolbox/cron-kubernetes" | ||
| spec.license = "MIT" | ||
| spec.required_ruby_version = ">= 3.2" | ||
| spec.required_ruby_version = "~> 3.4" |
There was a problem hiding this comment.
Unless it's absolutely required to accomplish the goals in this PR, I'd recommend separating out ruby version changes
| # Requires supporting ruby files with custom matchers and macros, etc, | ||
| # in spec/support/ and its subdirectories. | ||
| Dir[File.expand_path("support/**/*.rb", __dir__)].sort.each { |f| require f } | ||
| Dir[File.expand_path("support/**/*.rb", __dir__)].each { |f| require f } |
There was a problem hiding this comment.
Sorting files in the support directory is fairly standard practice and can help manage order dependencies in these files. Why was this removed?
| context "when RAILS_ENV is defined" do | ||
| before do | ||
| @rails_env = ENV["RAILS_ENV"] | ||
| @rails_env = ENV.fetch("RAILS_ENV", nil) |
There was a problem hiding this comment.
Changing from bracket to fetch("KEY", nil) seems unnecessary and additional characters for no additional benefit. What's the reasoning here?
This originated, because we needed to control these options:
Specifically the concurrency policy to avoid export or import jobs to be run in parallel (when new jobs kicks in before the old one is finished).